-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add webflow for importing bookmarks from Google #6707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add webflow for importing bookmarks from Google #6707
Conversation
402a647
to
f0c110e
Compare
3333dc7
to
2621c66
Compare
7afffb4
to
d7ee151
Compare
2621c66
to
eda2570
Compare
afcb488
to
072a68b
Compare
072a68b
to
a3608d0
Compare
a3608d0
to
b84aeb9
Compare
262315f
to
569981b
Compare
.../duckduckgo/autofill/impl/importing/takeout/webflow/ImportGoogleBookmarksWebFlowViewModel.kt
Show resolved
Hide resolved
.../duckduckgo/autofill/impl/importing/takeout/webflow/ImportGoogleBookmarksWebFlowViewModel.kt
Outdated
Show resolved
Hide resolved
00d8ed5
to
bb529d0
Compare
1b6e8bb
to
01d5515
Compare
4dd4efd
to
ca99169
Compare
01d5515
to
1dfcbd0
Compare
b217c11
to
0003be7
Compare
a714667
to
a8e4b48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 26 out of 31 changed files in this pull request and generated 5 comments.
...fig/privacy-config-api/src/main/java/com/duckduckgo/privacy/config/api/PrivacyFeatureName.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/duckduckgo/autofill/impl/importing/takeout/zip/TakeoutBookmarkExtractor.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/duckduckgo/autofill/impl/importing/takeout/zip/TakeoutBookmarkExtractor.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/duckduckgo/autofill/impl/importing/takeout/zip/TakeoutBookmarkExtractor.kt
Outdated
Show resolved
Hide resolved
5f1c36c
to
9b60bed
Compare
74686f2
to
d5ec240
Compare
...fig/privacy-config-api/src/main/java/com/duckduckgo/privacy/config/api/PrivacyFeatureName.kt
Outdated
Show resolved
Hide resolved
d5ec240
to
00f7fb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow works great 👍
Nothing to flag, great work!
} | ||
} | ||
|
||
private fun isTakeoutZipDownloadLink( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is an OR here, but I wonder if something should be AND. For example, url.contains(".zip") should be true always right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok with OR
as we don't want to be too prescriptive that we miss the download. Really, we should only hit the download flow for a takeout zip and nothing else so the guards are just a light touch in terms of validating. If the URL changes to not include zip
for instance, but it was still a takeout zip to download we should still continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, ok
entry = zipInputStream.nextEntry | ||
} | ||
|
||
return ExtractionResult.Error(Exception("Chrome/Bookmarks.html not found in file")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about this, from observability PoV. Should we differentiate between Errors vs We extracted but no file found?
Errors I see them as something went wrong, but here is User doesn't have bookmarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be if Chrome bookmarks data wasn't chosen for the export, not that they don't have any bookmarks to export so I think this is is a valid error
scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I thought it could happen on accounts without bookmarks or similar. What you describe, with the automated flow, that makes sense then. As if it's unchecked could indicate auto-flow broken.
Not sure what the real behavior will be on production, if the user will be able to interact with the page, even if it's automated. If that happens, they could uncheck bookmarks and check something else. Just something to consider if it's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the user will be able to interact with the page, even if it's automated
It won't be possible as the WebView will only show when user interaction is required to sign in / authenticate. A WIP demo available here: https://app.asana.com/1/137249556945/task/1210496756809250/comment/1211529725151990?focus=true
Task/Issue URL: https://app.asana.com/1/137249556945/project/608920331025315/task/1211197010246724?focus=true
Description
Adds the core webflow to support a guided bookmark import based on Google Takeout.
Autofill Dev Settings
screen forinternal
builds to testSteps to test this PR
First time import
internal
build type from this branchSettings->Autofill Dev Settings
Launch Bookmarks import flow
View Bookmarks
and verify they imported under aImported from Chrome
folderSubsequent import (already signed in)
Launch Bookmarks import flow
again. This time, you're already signed so verify it jumps straight into the automation (and that it succeeds)View Bookmarks
that there is only oneImported From Chrome
folder (bookmarks will be duplicated within that folder (it's known we're matching existing bookmark importer behaviour within that folder, including not further de-duplicating so if you import n times you'll see n sets of copies of bookmarks in that one folder)Check autofill works
Google Account Logout
(from theimport passwords
section) to logout of GoogleLaunch Bookmarks import flow
again.